Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BUILD_TESTS CMake option #66

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

Pravila00
Copy link
Contributor

Context

  • I am trying to update kaitai_struct_cpp_stl_runtime port in Vcpkg repository to the latest version, which supports creating a static library.
  • PR in Vcpkg repository.

Current issue

  • It seems like right now, for using the CMakeLists.txt file, it is mandatory to install GTest as well. Check CMakeLists.txt which is included always.
  • Given that at Vcpkg and even users may not be interested in building the tests (which forces having GTest), I suggest adding a CMake option for allowing to skip the build of tests.

@GreyCat
Copy link
Member

GreyCat commented Oct 5, 2023

Looks good to me, and it doesn't affect anything specifically here, so I believe we can merge this straight away.

@GreyCat GreyCat merged commit 1f92566 into kaitai-io:master Oct 5, 2023
3 checks passed
@generalmimon
Copy link
Member

generalmimon commented Oct 5, 2023

I wonder if doing this instead wouldn't be a better practice (https://cliutils.gitlab.io/modern-cmake/chapters/testing.html):

When you add your test folder, you should do something like this:

if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING)
    add_subdirectory(tests)
endif()

The reason for this is that if someone else includes your package, and they use BUILD_TESTING, they probably do not want your tests to build.

On the other hand, BUILD_TESTS was added as a project option here, so it should be at least overridable.

And BTW, BUILD_TESTING is apparently a much more common name than BUILD_TESTS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants